Fix down arrow key not working after font size change#364
Fix down arrow key not working after font size change#364objectiveous wants to merge 2 commits intoCodeEditApp:mainfrom
Conversation
The cached _estimateLineHeight in TextLayoutManager was never invalidated when the font changed. The vertical cursor movement calculation uses this estimate to compute the target y-coordinate, and after enough font size increases the stale (too small) value prevented moveDown: from crossing into the next line. Up arrow was unaffected because subtracting from the line top always lands in the previous line. Fix: re-assign renderDelegate after font/lineHeight changes to trigger its didSet which clears the cached estimate. Also handle arrow keys explicitly in the event monitor and add Ctrl+N/P (moveDown/moveUp) to handleCommand for robustness. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
0xWDG
requested review from
Copilot and
thecoolwinter
and removed request for
thecoolwinter
There was a problem hiding this comment.
Pull request overview
Fixes a cursor-navigation regression where vertical movement via Up/Down arrows stops crossing line boundaries after font size or line-height changes, by forcing the layout manager’s cached line-height estimate to refresh and by explicitly dispatching arrow-key handling when a local event monitor is installed.
Changes:
- Refresh
TextViewLayoutManager’s cached estimated line height after font and line-height multiplier updates. - Add explicit Up/Down arrow key dispatch (including common modifier combinations) in the local key event monitor path.
- Add Emacs-style vertical movement bindings: Ctrl-N (down) / Ctrl-P (up).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
Sources/CodeEditSourceEditor/SourceEditorConfiguration/SourceEditorConfiguration+Appearance.swift |
Forces layout-manager cache refresh after font/line-height changes to restore correct vertical cursor navigation. |
Sources/CodeEditSourceEditor/Controller/TextViewController+Lifecycle.swift |
Routes arrow keys through a dedicated handler and adds Ctrl-N/Ctrl-P vertical movement shortcuts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Force the layout manager to recalculate its cached estimated line height. | ||
| // The estimate is cached and not invalidated by font changes, causing vertical | ||
| // cursor movement (moveDown:) to use stale values and fail to cross line boundaries. | ||
| let renderDelegate = controller.textView.layoutManager.renderDelegate | ||
| controller.textView.layoutManager.renderDelegate = renderDelegate |
There was a problem hiding this comment.
The layout-manager cache invalidation is implemented by reassigning renderDelegate to itself. This works but is duplicated (and relies on a non-obvious side effect). Consider extracting this into a small helper (e.g., refreshEstimatedLineHeightCache() on the controller/layout manager) so the intent is clearer and the logic is not repeated.
| // Also invalidate the cached estimated line height (same issue as font change above). | ||
| let renderDelegate = controller.textView.layoutManager.renderDelegate | ||
| controller.textView.layoutManager.renderDelegate = renderDelegate |
There was a problem hiding this comment.
Same as the font-change branch above: the renderDelegate self-reassignment cache invalidation is duplicated. Please centralize this into a helper to avoid repeating the same workaround in multiple places.
| case (controlKey, "n"): | ||
| self.textView.moveDown(nil) | ||
| return nil | ||
| case (controlKey, "p"): | ||
| self.textView.moveUp(nil) | ||
| return nil |
There was a problem hiding this comment.
The new Ctrl-N / Ctrl-P shortcuts match only when modifierFlags equals exactly .control. Because modifierFlags is built from .deviceIndependentFlagsMask, it can include extra flags (e.g. Caps Lock / Fn / numericPad), which would prevent these bindings from firing. Consider normalizing the flags to only [.shift, .control, .option, .command] before switching, or using .contains(.control) with explicit exclusions for other modifiers as needed.
| /// Handles up/down arrow key events with all modifier combinations. | ||
| /// Dispatches the appropriate movement method on the text view and consumes the event. | ||
| /// | ||
| /// - Returns: `nil` to consume the event after dispatching the movement action. |
There was a problem hiding this comment.
Doc comment says this handles arrow keys "with all modifier combinations", but the implementation only dispatches for Shift/Option/Command combinations and returns the event for others (e.g. Control+Arrow). Please adjust the wording to match what is actually handled so readers don’t assume broader coverage.
| /// Handles up/down arrow key events with all modifier combinations. | |
| /// Dispatches the appropriate movement method on the text view and consumes the event. | |
| /// | |
| /// - Returns: `nil` to consume the event after dispatching the movement action. | |
| /// Handles up/down arrow key events for plain, Shift, Option, Command, | |
| /// and their combinations among these modifiers. | |
| /// Dispatches the appropriate movement method on the text view and consumes the event. | |
| /// | |
| /// - Returns: `nil` to consume the event after dispatching the movement action, | |
| /// or the original event for unsupported modifier combinations. |
| /// | ||
| /// - Returns: `nil` to consume the event after dispatching the movement action. | ||
| private func handleArrowKey(event: NSEvent, modifierFlags: NSEvent.ModifierFlags) -> NSEvent? { | ||
| let isDown = event.keyCode == 125 |
There was a problem hiding this comment.
handleEvent defines downArrow/upArrow constants, but handleArrowKey re-hardcodes 125 to compute isDown. To reduce magic numbers and keep key-code mapping consistent, reuse the same constants (or centralize them) instead of duplicating the raw value.
| let isDown = event.keyCode == 125 | |
| let downArrow: UInt16 = 125 | |
| let upArrow: UInt16 = 126 | |
| let isDown = event.keyCode == downArrow |
- Extract renderDelegate cache invalidation into refreshEstimatedLineHeightCache() helper - Normalize modifier flags to [shift, control, option, command] so Ctrl-N/P work with Caps Lock/Fn - Fix handleArrowKey doc comment to accurately describe supported modifier combinations - Centralize key code constants (tab, downArrow, upArrow) as static properties Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Extract renderDelegate cache invalidation into refreshEstimatedLineHeightCache() helper - Normalize modifier flags to [shift, control, option, command] so Ctrl-N/P work with Caps Lock/Fn - Fix handleArrowKey doc comment to accurately describe supported modifier combinations - Centralize key code constants (tab, downArrow, upArrow) as static properties Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Description
After changing the font size, the down/up arrow keys stop moving the cursor across line boundaries. The root cause is that
TextViewLayoutManagercaches an estimated line height that isn't invalidated when the font changes. ThemoveDown:/moveUp:methods use this stale value to calculate the target point, which lands within the same line instead of the next one.Fix:
renderDelegateon the layout manager after font or line height multiplier changes, which forces the cached estimate to refresh.handleArrowKeydispatch so arrow keys with all modifier combinations (Shift, Option, Cmd) are handled correctly when the local event monitor intercepts them.Related Issues
Checklist